Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented better way to map inputs in godot #16797

Closed
wants to merge 3 commits into from

Conversation

AndreaCatania
Copy link
Contributor

With this commit I've added a way to map stick (axis) input inside Godot actions.

Basically now is possible to map together axis and buttons in order to simplify the life of developer and allow to change bindings at runtime easily.

Please check this video to see it in action: https://youtu.be/tKd7rEfwpIo

@vnen
Copy link
Member

vnen commented Feb 18, 2018

Related to #14482 and #14641.

@groud
Copy link
Member

groud commented Feb 18, 2018

I don't understand why people want dealing with axis instead of actions.
To make this simple I would just define a get_action_value() that returns a value between 0 and 1 telling how "strong" is the action pressed. So that you can map a single action both to a single key and a single axis+direction.

@leoddd
Copy link
Contributor

leoddd commented Feb 18, 2018

This is really useful. Would simplify a lot of my personal input handling code!

I would agree with groud in that naming it a little more generically just get_action_value might be better, since this actually has uses outside of strict joystick axis simulation (even if it isn't 100% necessary for that).

@AndreaCatania
Copy link
Contributor Author

@groud I like get_action_value so if others like it too, I will rename it

@groud
Copy link
Member

groud commented Feb 18, 2018

It's a little bit more than a renaming, it's just avoiding replacing the (axis+direction) way of mapping by an axis (without direction) mapping. While it work pretty well by now.
Also that would make it compatible with analog buttons or analog keyboard key.

@AndreaCatania
Copy link
Contributor Author

Oh.. in this case I've already did it, check this Screen Shot
screenshot from 2018-02-18 22-26-42

@BastiaanOlij
Copy link
Contributor

BastiaanOlij commented Feb 18, 2018

I also like Grouds idea for the name.

Love this change @AndreaCatania

Groud, do note that its not just the mapping, you have to define whether a keypress results in a positive or negative value so I like what Andrea has done here.

@groud
Copy link
Member

groud commented Feb 18, 2018

Oh.. in this case I've already did it, check this Screen Shot

Precisely, this is what I don't want, but the contrary. The previous system allowed to bind both an axis and a key to the same action with no more complex stuff.

@AndreaCatania
Copy link
Contributor Author

With this PR you can bind key and axis to the same action, you can know the action axis and also if it's pressed or released.

Can you please give me an example in order to understand exactly what you mean?

@groud
Copy link
Member

groud commented Feb 18, 2018

Well, by now we have this:
2018-02-18-225637_389x243_scrot
Which works fine but just requires something to retreive how "strong" is the action. So what i suggest is adding a get_action_value() telling how "strong" is the action when pressed.

So in code you would do:

if (is_action_pressed("my_action")):
   velocity += get_action_value("my_action") * SPEED

Where get_action_value returns something between 0 and 1 depending on how strong is the action.
(and always 1 when the action is used with a keyboard key).

If you bind the action to a negative axis, the value from get_action_value is still between 0 and 1 by getting the absolute value of the axis.

@BastiaanOlij
Copy link
Contributor

@groud the downside of that approach is that you'd need to do:

motion.y -= get_action_value("move_up")
motion.y += get_action_value("move_down")

While @AndreaCatania solution would allow you to do:

motion.y = get_action_value("move_y")

That said, I don't mind the extra line of code to combine them and it would fit well in the current input mapping making the change far less impactful. For a lot of already made games you can leave everything as is but just changing get_action_pressed for get_action_value where you need analogue precision.

With for instance the car simulation I'm working on throttle and brake could be one joystick axis (gamepad play) or two trigger axis (steering wheel and peddles) so the breakup into a separate throttle and breaking action makes far more sense.

So yeah, I think Grouds suggestion would work out well.

@mysticfall
Copy link
Contributor

mysticfall commented Feb 18, 2018

It looks nice and seems to be just what I want for my project.

By the way, does it also support using mouse motion or wheel as an axis, or do we need another issue/PR to implement that?

And does an axis input repeats its value when it's in the 'neutral' position, or does it work the same way as a trigger type input (emits events only when it's 'pressed')?

@groud
Copy link
Member

groud commented Feb 18, 2018

Sure, it leads to more lines of code, but it's more consistent in the binding as a key cannot have a negative value. Else it would mean that you would have actions that can go into negative values and other that can't. Even if it's a little bit counter-intuitive at the beginning, it means any action can be binded to any type of input, which is, IMHO, easier to manage in the end :)

@BastiaanOlij
Copy link
Contributor

@groud thing is, the current solution does not have a direction for key (pressed is always 1) but you have a direction for axis which is why every axis appears twice. Its not just taking the absolute of the axis, it is "for a positive axis, return the value as if when positive else 0" and "for a negative axis, return the values negated if negative else 0". Andrea just reversed that, returning the axis value as is, but allowing keys to return either 1 or -1 or assumably any value which can be handy.

But don't get me wrong, the more I think of it, the more I believe your suggestion is the better one. Right now we have a separate implementation for action pressed and action value. Your suggestion might require an extra line of code, it offers more flexibility and its simpler to understand. It also prevents confusion between people who want to use actions for normal key pressed actions and for axis input.

That all said, I had a quick look through the code and this change could take a bit of work. I would suggest adding a value to the Action structure and then enhancing this loop:
https://github.com/godotengine/godot/blob/master/main/input_default.cpp#L332

To not just set action.pressed to true but also action.value to the value we want. It'll mean enhancing the event class to return the value for that event, which for a key event would be 0 or 1, for a joystick event would be between 0.0 and 1.0 and for mouse event i guess based on the relative mouse movement?

It also means adding a 2nd parameter for the value to:
https://github.com/godotengine/godot/blob/master/main/input_default.cpp#L469
And making sure that defaults to 1.0 for backwards compatibility

And making sure:
https://github.com/godotengine/godot/blob/master/main/input_default.cpp#L480
Sets it to 0.

All in all it does feel like an elegant solution if done right.

@AndreaCatania
Copy link
Contributor Author

AndreaCatania commented Feb 18, 2018

I don't understand what is the benefit to have a value between 0 and 1, since with the approach that you said you have to create always an action for each axis direction and also you have to bind keys to these.

Also you have to check for each axis direction if the action occurs, and this is useless for most of the times.

I don't know, but I don't see a lot of situation where split the two axis in four other make sense, or makes life easier, even because in order to support it I've to add more checks to it, and I don't like it.

I think that at the end the most of the times you will end with a GDScript like this:

motion.y -= get_action_value("move_up")
motion.y += get_action_value("move_down")
motion.x -= get_action_value("move_right")
motion.x += get_action_value("move_left")

plus all code to split these in the engine.

It add useless complexity, so for me it doesn't worth and doesn't make the life easy

@BastiaanOlij
Copy link
Contributor

BastiaanOlij commented Feb 18, 2018

@AndreaCatania When you look at character movement in a top down map, you are absolutely right. But in many other use cases you'll fine your self doing this:

if get_action_Value("move_y") < 0.0:
  do_crouch(true) #make sure character is crouched
else:
  do_crouch(false) # make sure character stands up

if get_action_value("move_y") >= 0.0:
  do_jump(get_action_value("move_y")) # jump with input relating to strength of jump

Instead I can do:

do_crouch(get_action_pressed("crouch"))
do_jump(get_action_value("jump"))

For my brake example it gets even worse. I would have to do something like:

if i_have_a_gamepad:
  if get_action_value("throttle") >= 0.0:
    accelerate(get_action_value("throttle"))
  else:
    brake(-get_action_value("throttle"))
else:
  accelerate(get_action_value("throttle"))
  brake(get_action_value("break"))

It may seem weird to handle both accelerate and brake actions at the same time if you have separate axis for them but anyone who has driven go-karts knows how much fun using them at the same time can be :) You can even see these are separate properties in the physics engine

The code simplifies to:

  accelerate(get_action_value("throttle"))
  brake(get_action_value("break"))

So yes, I fully agree with you, for normal top down games where you just need a value between -1.0 and 0.0 for character movement, you need two lines of code where one will do. But at the added price of far more setup in the input mapping.

There is no right or wrong here imho. Both have pro's and con's. What I like about Grouds suggestion is that it doesn't change anything in the input mapping, it just adds to the information you can gather about the state of controllers. I also feel it makes it easier to setup more complex control situation such as my acceleration/braking scenario where it is more likely you're dealing with two separate axis.

@groud
Copy link
Member

groud commented Feb 18, 2018

@AndreaCatania Yeah, but this solution complexifies the mapping system by introducing actions that can have two different directions. In you case, if you want to map a key to a double-direction action, you have to map two keys under the action. As an example, for your move_x action you would have to map the left key as negative and the right key as positive, while both are under the same action. It makes the input mapping confuse just for removing 2 lines of code. While, let's be honest, you usually have those lines only once in your code.
Thus I don't think the gain in code lines worth the (relative) complexity it adds to the input mapping. To be honest, I found the current system a little bit weird at the beginning, but I find it a lot more consistent and finally easy to understand.

@BastiaanOlij
Copy link
Contributor

btw

motion.y = get_action_value("move_down") - get_action_value("move_up") # in 2D, other way around in 3D ;)
motion.x = get_action_value("move_right") - get_action_value("move_left")

= win :)

@reduz
Copy link
Member

reduz commented Feb 18, 2018 via email

@groud
Copy link
Member

groud commented Feb 18, 2018

If this is just for axis, api should be get_axis_value and we should have a different category for naming and remaping axes, instead of trying to fit it to the action system IMO

We already have something to retreive an axis but it does not solves the initial problem of having to deal with code for actions (=keys pressed or not) and joysticks split into two. I think my solution solve the problem pretty well, but well, I'm not unbiased. ^^

@BastiaanOlij
Copy link
Contributor

@reduz having just added both keyboard support and axis support to my demo racing game (and it being indirectly responsible for @AndreaCatania to spend time on this I think) I would beg to differ. I think enhancing the action system is worth considering as it would simplify a lot of coding. I do like @groud suggested approach most so far :)

@reduz
Copy link
Member

reduz commented Feb 18, 2018 via email

@BastiaanOlij
Copy link
Contributor

BastiaanOlij commented Feb 18, 2018

But an action is an action, it's something that happened at a given time.
This is not an action, so it's confusing. I prefer it's named according to
what it really is.

@reduz I think that is the problem. Semantically you are absolutely correct. This change makes it no longer purely an action based thing as in "if the user triggers action A, B should happen". But when you're looking at it from a practical perspective, even today it is already much more then that. Look at this code fragment that is in nearly every Godot platform game:

func _process(delta):
  if Input.get_action_pressed("move_left"):
    position.x = position.x - (delta * MOVE_SPEED)
  elif Input.get_action_pressed("move_right"):
    position.x = position.x + (delta * MOVE_SPEED)

That already is "not an action". We're already "abusing" the action system here because practically speaking the action system allows us to define keyboard, mouse and joystick mappings in a way that we can write code that doesn't make us worry about the input device the user is using.

The case may thus be that the action system is wrongly named not that this small change is out of place. While it is used for action based events ("a jump action triggering a jump event which initiates the jump action") it is used for so much more.

@reduz
Copy link
Member

reduz commented Feb 18, 2018 via email

@BastiaanOlij
Copy link
Contributor

But this was never a purely action based system, it's part of the Input
system.

@reduz exactly, so what is wrong with adding the ability to not just get the on/off is_pressed value of mapped input but also get a value between 0.0 - 1.0 to turn the input map into something you can use for analogue input?

It seems convoluted to me to build a new mapping system for analogue input when the system we have provides us with 95% of what we need.

Or am I barking up the wrong tree and we're actually talking about the same thing, you just want the name of the function to be different?

@BastiaanOlij
Copy link
Contributor

@reduz note btw, that this is not just for axis, the whole goal here is to map an action like "move forward" to a key (W), an axis (axis 1 positive) and maybe even mouse movement, exactly like the current mapping already does, but instead of getting a true/false, we get a value between 0.0 and 1.0. When the key is pressed it will return 1.0, if the joystick is used, the value is between 0.0 and 1.0

@AndreaCatania
Copy link
Contributor Author

AndreaCatania commented Feb 19, 2018

Too much messages to reply 😱
so:

First thing is that I never said that I've removed the old actions APIs (that would be a stupid thing to do IMO since the actions APIs are fine) So this is the list of supported API in this moment with this PR

  • is_action_just_pressed
  • is_action_just_released
  • is_action_pressed
  • is_action_released
  • is_action_just_changed
  • get_action_axis_value

As you can see I've only added two APIs this mean that all old approach still valid and correct to use, but in addition you will have (if required) the possibility to get value of the axis.

For example now is possible to have a Joypad where the buttons has a pressure, now we can map it easily.

Reply:

@AndreaCatania When you look at character movement in a top down map, you are absolutely right. But in many other use cases you'll fine your self doing this:
[ETC......]

@BastiaanOlij As I've said above you can and you MUST continue to use this approach:

do_crouch(get_action_pressed("crouch"))
do_jump(get_action_value("jump"))

This PR has the only objective to add a way to map Sticks inside the current actions in order to have the same mapping for all kind of devices.
With this you will have the benefit to centralize all the code to handle inputs inside one function
You can change button binding at runtime without care about device type
You have less actions to handle.

@AndreaCatania Yeah, but this solution complexifies the mapping system by introducing actions that can have two different directions. In you case, if you want to map a key to a double-direction action, you have to map two keys under the action. As an example, for your move_x action you would have to map the left key as negative and the right key as positive, while both are under the same action. It makes the input mapping confuse just for removing 2 lines of code. While, let's be honest, you usually have those lines only once in your code.
Thus I don't think the gain in code lines worth the (relative) complexity it adds to the input mapping. To be honest, I found the current system a little bit weird at the beginning, but I find it a lot more consistent and finally easy to understand.

@groud You are saying that this solution makes things confusing and less consistent, but I never saw a single post that complain this approach to UE4, and also people seems happy for this implementation on YouTube and also Twitter. I think that this approach makes things a lot easier.

Doesn't exists a single game, or at least very few, where you get a benefit to separate the two axis of joystick in other four. The sticks are always used to move the camera position or orientation and the range -1 to 1 is perfect for this scenario.

Also I want to talk about range of inputs, with this PR you will get a different range according to the input, for example as discussed before if you have a Stick the axis range will be -1 to 1 but if you have a shoulder trigger (for example L2) the range will be 0 to 1. So the function get_action_axis_value will return always the natural value that you expect, or 0 if the button doesn't simulate the axis.

But an action is an action, it's something that happened at a given time. This is not an action, so it's confusing. I prefer it's named according to what it really is.

@reduz I thing that move a stick is also an action, and you can use is_action_just_changed to know if this action is just triggered or not.


However I would be glad if you compile the code and you test it with your current project and then would bring to me the problems that you have found, so I can try to figure out how I can improve it

@BastiaanOlij
Copy link
Contributor

Just a few more cents from my side after talking somewhat more in depth with Odino to kick ideas around and understand his reasoning better.

I think Odino's approach does make more sense to me now but I see the sense in a number of the things raised especially in it potentially being confusing in the action system.

I think Reduz probably highlighted this best when we discussed if this should be part of the action system. While there is overlap in this all being about input, in the long run this is about what makes sense for building a game that uses analogue input with a fallback to a keyboard.

So here is my suggestion for making this better:

  1. split the interface, make it clear that the action mapping is different from axis mapping even though they are similar. Either a separate tab or two sections on the input tab.
    Actions are about singular events. The user presses W, the user presses Spacebar, and mapping that to equivalent joystick actions.
    Axis mapping is about getting named access to axis on a joystick with the ability for the user to change that mapping if they have different controllers and with a fallback to keyboard input that simulates axis. These are not event driven actions but purely state driven.
  2. call the field in which you put -1.0 or 1.0 scale to indicate you apply a scale to this input. I liked this about the screenshot I was shown about UE4. I see sense in being able to put other values in here. D could be mapped to 0.5 instead of 1.0. Pushing your stick all the way to the right means the character moves at its fastest which may not be what you want with keyboard input because you don't have the control over speed.
  3. don't use action in the names of the new functions. Call them has_axis_changed and get_axis_value. They are not action based.

Just my take on it at this point in time :)

@aaronfranke

This comment has been minimized.

@groud
Copy link
Member

groud commented Mar 13, 2018

Well yeah, having to write something like motion.y = get_action_value("move_down") - get_action_value("move_up") is one of the flaws we mentioned. However, I believe it worth it since:

  • You usually handle inputs only in a single place in your game
  • It avoids confusing people when then have to choose between Axes and Action, if they are not sure about the gameplay they want.
  • It avoid creating a new way of handling Inputs in general, while still providing the exact same features.

But yeah, finally the question is whether if we want users to write a little bit more code, or if we want them to spend more time in the Axes/Actions menu. I personnaly hate having to remap inputs when my gameplay changes, that's why I prefer my solution.

@aaronfranke
Copy link
Member

aaronfranke commented Mar 13, 2018

I guess I am overestimating the benefits of a -1 to 1 range, something like "up minus down" does seem simple enough to program. Anyway, anything is better than booleans for my use case.

I would still like to state that, for keyboards, a get_action_value is only useful if we can control the "gravity" etc of values (rate to go towards 1 when pressed, rate to drop back to 0 when released)

@groud
Copy link
Member

groud commented Mar 13, 2018

I would still like to state that, for keyboards, a get_action_value is only useful if we can control the "gravity" etc of values (rate to go towards 1 when pressed, rate to drop back to 0 when released)

Sure, that kind of things could be modified in the InputMap menu.

Anyway, to be honest this seems to be something quite controversial. I believe we should organize a vote or something like that, to see what people think about both idea.

@AndreaCatania
Copy link
Contributor Author

@aaronfranke

Another idea occured to me, we could have everything be axis values internally, but have a boolean method that returns true if > 0.1. Maybe even a "neg direction" check if < -0.1. But 0.1 could be anything, maybe 0.5 is better? This would allow you to map inputs one way but still easily pull out booleans.

Currently this implementation does it

@HummusSamurai
Copy link
Contributor

HummusSamurai commented Mar 16, 2018

I strongly prefer the solution in #16902 by @groud to this one, as I find it more consistent with the minimalistic philosophy of the engine and allows for the most flexibility.

@esijg
Copy link
Contributor

esijg commented Mar 23, 2018

Just wanted to say that this is exactly what I need and would save me a lot of work with my game Vegg. Both implementation concepts of @AndreaCatania and @groud seem to be good and even though one would fit better for my current game the other one would require just minimal workarounds. And for other games it would be the other way around.

Might even port my game from Godot 2 to Godot 3 if this gets merged.

@AndreaCatania
Copy link
Contributor Author

In the last commit I've improved input mapping for multiplayer games by adding per controller mapping. Check this video and let me know what do you think about: https://youtu.be/vCBu6CSsnjo

@ghost ghost added the topic:input label Apr 7, 2018
@akien-mga
Copy link
Member

Why was it closed? (Superseded by another PR I guess? But it would be good to mention here for clarity)

@AndreaCatania
Copy link
Contributor Author

AndreaCatania commented Apr 9, 2018

Akien ye correct, I closed it because reduz like more the approach of splitting joy axis, so this #16902 PR will be merged

@QbieShay
Copy link
Contributor

QbieShay commented Apr 9, 2018

For what's worth it, i prefer having axis mapped from -1 to 1. I don't see the use case for [0,1]. I actually think it would be annoying to work with.

@raymoo
Copy link
Contributor

raymoo commented Apr 9, 2018

[0,1] makes sense for throttle or trigger inputs, but of course [0,1] fits within [-1,1]

@AndreaCatania
Copy link
Contributor Author

@raymoo In my solution the shoulders trigger are mapped 0 - 1 and usually are used for throttle in car games. So as i said before no real need to separate the axis.

@Dudemanjude
Copy link

What about the multiplayer controller mapping? I like the per controller mapping, it would be very useful for making local multiplayer games.

@AndreaCatania
Copy link
Contributor Author

AndreaCatania commented Apr 23, 2018

I like it too, but reduz said that is too simple work around it that is useless implement it.

@groud
Copy link
Member

groud commented Apr 23, 2018

What about the multiplayer controller mapping? I like the per controller mapping, it would be very useful for making local multiplayer games.

Actually, you can map and define per-player actions by doing something like that:

  • You create a set of actions to be mapped for each player
  • You duplicate the action for each player via code, while renaming them by using something like "jump_PlayerID", "shoot_PlayerID"... (you change the player ID)
  • You check each action using something like Input.is_action_pressed("jump_"+player_id)

@Dudemanjude
Copy link

I know you can do that, but it still would be nice to have it done semi-automatically like it does in this PR. I've done that sort of mapping before whenever I've tried to implement local multiplayer. It seemed like a pretty nice addition, without having to manually type in all of the names, but it's not that hard to work around it.

@aaronfranke
Copy link
Member

Now that the other PR for inputs has been merged, it would probably be best to open a new PR that only includes changes we'd like to make on top of it.

@Xrayez
Copy link
Contributor

Xrayez commented Feb 23, 2020

Regarding multiplayer controller mapping per player thing...

What about the multiplayer controller mapping? I like the per controller mapping, it would be very useful for making local multiplayer games.

Actually, you can map and define per-player actions by doing something like that:

  • You create a set of actions to be mapped for each player
  • You duplicate the action for each player via code, while renaming them by using something like "jump_PlayerID", "shoot_PlayerID"... (you change the player ID)
  • You check each action using something like Input.is_action_pressed("jump_"+player_id)

I've proposed a somewhat refactored approach regarding how input states can be decoupled from input polling system here: godotengine/godot-proposals#104.

The proposal is centered around having an easier way to control multiple characters within a game session, not necessarily multiple players per session, but there may be some connections which I can't see for myself being useful what was proposed here.

Talking specifically, my proposal aims to provide some encapsulated input state which can basically reflect the global input state the same way you'd use an Input singleton, but also having an InputState resource which can be synced and/or shared between player/character instances. What I imagine could be useful is assigning some special controller mapping identifier within each state so that the input polling system can retrieve/update only those states which are actually relevant/matches the identifier.

My current implementation #35240 only has a master instance of an InputState. What could possibly be useful is actually dynamically allocating the necessary input states whenever you have more than one controller being connected. Compare:

# Local single-player
var state = Input.state as InputState
var player = state.get_id()
# ... input handling code for the player goes here ...
if state.is_action_pressed("shoot"):
    pass

vs

# Local multi-player
for i in Input.get_local_state_count():
    var state = Input.get_local_state(i) as InputState
    var player = state.get_id()
    # ... input handling code for EACH player/controller goes here ...
    match player:
        0:
            if state.is_action_pressed("shoot"):
                shoot_apples()
        1:
            if state.is_action_pressed("shoot"):
                spam_eggs()
     # etc.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.